Conversation
github-actions
Bot
added
sqllogictest
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24817 +/- ##
==========================================
+ Coverage 81.53% 81.71% +0.18%
==========================================
Files 1123 1127 +4
Lines 406042 416238 +10196
Branches 406042 416238 +10196
==========================================
+ Hits 331059 340147 +9088
- Misses 55621 56100 +479
- Partials 19362 19991 +629 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. The all-or-nothing eligibility check makes sense and addresses the correctness issue with mixed aggregate expressions.
I have one blocking comment around the regression coverage. The current test confirms that the dynamic filter is no longer present in the plan, but I think we should also verify the query result directly so that the test protects the user-visible behavior this change is fixing.
I also left one small, non-blocking naming suggestion.
| # -> DynamicFilter [ a < 1 OR a > 8 OR b > 12 ] (MIN(c+1) dropped as unsupported) | ||
| # No dynamic filter because not every aggregate has a safe predicate. | ||
| query TT | ||
| EXPLAIN ANALYZE SELECT MIN(a), MAX(a), MAX(b), MIN(c + 1) FROM agg_dyn_mixed; |
There was a problem hiding this comment.
Could we add an execution-result assertion here as well? The current EXPLAIN ANALYZE assertion confirms that the dynamic filter is absent, but it does not directly verify the correctness issue this change is intended to fix: the unsupported aggregate still needs to see every relevant row.
For this fixture, the expected result for MIN(a), MAX(a), MAX(b), MIN(c + 1) is 1, 8, 12, 71.
It would also be good to make the fixture or scan order deterministic enough that the old behavior actually prunes the row needed by MIN(c + 1). With the current two-file layout, that can depend on which file publishes its bounds first. Ideally, this regression test should fail with the old implementation because it produces the wrong result, rather than only because the plan contains a dynamic filter.
There was a problem hiding this comment.
Good point. I've added a result check and adjusted the test data so the old implementation produces the wrong result. The test passes with the fix.
| /// is enabled only when every aggregate expression is supported, so this vector | ||
| /// contains an entry for every accumulator. For example, `min(a), max(b)` produces | ||
| /// entries for `min(a)` and `max(b)`. | ||
| supported_accumulators_info: Vec<PerAccumulatorDynFilter>, |
There was a problem hiding this comment.
Small naming suggestion, not blocking: would it make sense to rename supported_accumulators_info to something like accumulator_dyn_filter_info? With the new all-or-nothing behavior, this contains metadata for every aggregate whenever dynamic filtering is enabled, so dropping supported might make that invariant a little clearer.
There was a problem hiding this comment.
Yes, that makes sense to me. renamed to accumulator_dyn_filter_info.
… and clarify naming
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
@lyne7-sc, thanks for addressing the earlier feedback. This looks good to me now.
The aggregate dynamic filter is correctly disabled when any aggregate is not a plain-column MIN or MAX, which avoids filtering out rows needed by unsupported aggregates. The rename to accumulator_dyn_filter_info is also applied consistently across the execution and validation paths.
I also appreciate the deterministic parquet regression coverage. The result assertion makes the failure mode clear, and the fixture demonstrates that the old partial-filter behavior would incorrectly produce MIN(c + 1) = 101 instead of 71.
No further comments from me. Thanks for the updates!
|
🚀 |
Adapted from apache#24817, commit b3cb365. Retain the equivalent 53.1.0 eligibility guard without unrelated metadata renames. Signed-off-by: discord9 <discord9@163.com>
Backport apache#24817 (b3cb365) to DataFusion 55. Signed-off-by: discord9 <discord9@163.com>
Backport apache#24817 (b3cb365) to DataFusion 55. Signed-off-by: discord9 <discord9@163.com>
…#24817) ## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123. --> - Closes apache#24816. ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. Please explain the problem you are trying to solve in terms of the user-visible behavior, rather than the implementation. For example, "The code in `foo.rs` doesn't handle nulls" is a symptom of the implementation. "COUNT(DISTINCT) returns wrong results when the column contains nulls" is the user-visible problem. --> Aggregate dynamic filtering currently ignores unsupported expressions while still generating a shared scan predicate from supported aggregates. This can prune rows required by the unsupported aggregate and produce incorrect results. ## What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here, but it is sometimes worth providing a summary of the individual changes in this PR. --> - Disable aggregate dynamic filtering when any aggregate expression is unsupported. - Document that aggregate dynamic filtering requires every aggregate expression to be supported. - Update the existing sqllogictest to verify that no dynamic filter is produced for mixed supported and unsupported expressions. ## What is the testing strategy for this PR? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code Briefly describe how this PR is tested, and point to the specific tests you added. For example: 'This new feature is covered by the `sqllogictest` cases added in `foo.slt`'. If this PR does not add tests, explain why. For example, if the change is already covered by existing tests, please mention it. You should also check the `codecov` bot reply on this PR to confirm the changed code is exercised. --> Yes. Updated the existing sqllogictest case in `push_down_filter_regression.slt`. ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please add the `api change` label. --> Yes. This fixes potentially incorrect query results. No API changes. (cherry picked from commit b3cb365) Conflicts resolved when applying to spiceai-54 (DataFusion 54.1): - datafusion/physical-plan/src/aggregates/mod.rs: the base spells the cols_for_dynamic_filter check as debug_assert!(a == b); applied only the upstream rename (supported_accumulators_info -> accumulator_dyn_filter_info) to that line. - datafusion/sqllogictest/test_files/push_down_filter_regression.slt: the base carries an older version of the mixed-expressions section; replaced it with the upstream section verbatim. The upstream aggregate_stream.rs hunks apply to aggregates/no_grouping.rs, its name on 54.1.
3 participants
Which issue does this PR close?
Rationale for this change
Aggregate dynamic filtering currently ignores unsupported expressions while still generating a shared scan predicate from supported aggregates. This can prune rows required by the unsupported aggregate and produce incorrect results.
What changes are included in this PR?
What is the testing strategy for this PR?
Yes. Updated the existing sqllogictest case in
push_down_filter_regression.slt.Are there any user-facing changes?
Yes. This fixes potentially incorrect query results. No API changes.